Ewm3645 mask workspace diffcal #658
Conversation
fix import path apparently the auto import logic has changed for the worse fix fetch diffcal groceries unit test odd update to try and get github to kick off ci add test for the combining of masks for diffcal actually pass the maskworkspace to be used by the backend update tests to accept maskworkspaces from different states as valid fix integration tests added a test to confirm resident masks are filtered if not compatible change exception type to something not pydantic up the last couple lines of coverage update precommit, remove commented out code fix mock method definition
…workspace_diffcal
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #658 +/- ##
==========================================
- Coverage 96.34% 96.25% -0.10%
==========================================
Files 77 77
Lines 7010 7052 +42
==========================================
+ Hits 6754 6788 +34
- Misses 256 264 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ekapadi
left a comment
ekapadi
left a comment
There was a problem hiding this comment.
I got through comments for everything but the tests. I'll go through those next. Most of these comments, with a few exceptions, are just nit-picking. However, they'll probably show a few things that you could help explain!
src/snapred/backend/recipe/algorithm/FitMultiplePeaksAlgorithm.py
Outdated
Show resolved
Hide resolved
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
49e6036 to
6499fcb
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
1bef35a to
b720b8c
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
94b6307 to
a87bd8f
Compare
for more information, see https://pre-commit.ci
ekapadi
left a comment
ekapadi
left a comment
There was a problem hiding this comment.
I had some issues with the way combinePixelMasks creates and uses its <output workspace>. I think I'd prefer that it would explicitly create it, not depending on any "it's the first workspace in the incoming list" type of behavior. The problem is that, in workbench, the user could be working on a bunch of different things at the same time. It is not OK that one of their masks suddenly dissappeared and was re-purposed for SNAPRed's purposes. :)
…of combinePixelMasks already exist
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
4026ef5 to
02fdbfe
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
52d1195 to
6fc6cf3
Compare
for more information, see https://pre-commit.ci
…e the initial host for offsets
for more information, see https://pre-commit.ci
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
bcefcbb to
41f773f
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
9527281 to
3bbab68
Compare
walshmm
force-pushed
the
ewm3645_mask_workspace_diffcal
branch
from
db1fabc to
85f2b96
Compare
ekapadi
left a comment
ekapadi
left a comment
There was a problem hiding this comment.
I ran these changes through extensive tests, using various types of input mask, including pathological cases. Several of these tests initially triggered failing edge cases, but all of these have since been fixed to my satisfaction. With the caveat as indicated below: I approve these changes.
During the course this testing, it was noted that there seems to be a rare case which triggers an "LHS != RHS" error (indicating a mismatch of masked group counts during the diagnostics calculation) -- unfortunately we could not then repeat this issue -- it seems to depend either on the specifics of the node it is run on, or possibly something about the history of the GUI actions. Defect EWM12329 "Diffcal Masking Inconsistencies", should cover this case.
Description of work
This pr enables users of snapred to supply a user generated pixel mask to be used as part of diffraction calibration.
Explanation of work
A lot of the work in this pr was around all the edge cases in the sub-algorithms that didnt account for fully masked groups.
The other bits of work involved orchestrating the mask, and combining it with the potentially existing diffraction calibration mask.
I also refactored the diffcal section of the happy path tests to be much more modular.
This way I was able to include an additional test with the same sequence of events, except with the addition of testing for user masks.
A user mask influences the following steps of the workflow, so it necessitated a whole new run.
To test
Observe what the new integration test does.
If you would like to test it manually, you will need to load your target input workspace, or one with the same instrument as it.
Then mask out a whole group, extract the mask, and name it
MaskWorkspace_2.(I think I need to write a defect, as I encountered
MaskWorkspace_1's not properly being sent along, though that may be my fault?)Now run diffcal with this mask selected, the tweak peaks view should have at least one less graph if you masked out a group.
Run to completion.
Link to EWM item
EWM#3645
Verification
Acceptance Criteria
This list is for ease of reference, and does not replace reading the EWM story as part of the review. Verify this list matches the EWM story before reviewing.